Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add method to multiply dense vectors with CSC matrices #809

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add method to multiply dense vectors with CSC matrices #809

wants to merge 2 commits into from

Conversation

aaronquantexa
Copy link

Hi, I saw this issue #715 and thought I'd have a go seeing as it's a good first issue (over 1 year old!).

I've copied the style of code around the repo and lightly cloned this test: "SparseVector * CSCMatrix Lifted OpMulMatrix & Transpose" to validate the matrix multiplication is correct for DenseVectors.

To allow the multiplication I've extracted 2 conversions (DenseVector to CSCMatrix and CSCMatrix to DenseVector - this latter one has an assertion inside that the Matrix has just 1 row), which are public.

Do let me know if there is more testing / other changes required.

@dlwh
Copy link
Member

dlwh commented Apr 29, 2021

Hi @aaronquantexa, thanks for the PR!

Two things:

  1. I think it makes more sense to get there via conversion from the DenseMatrix/CSCMatrix implementation from here

    breeze/math/src/main/codegen/breeze/linalg/operators/CSCMatrixOps.scala

    Line 679 in a1c5646

    implicit def canMulDM_M[@expand.args(Int, Float, Double, Long) T]
    rather than the CSC/CSC, since a Transpose[DenseVector] is conceptually just a DenseMatrix with 1 row.

  2. I'm in the middle of very large and obnoxious refactor to get Scala 3 support working, which has necessitated moving around a whole lot of stuff. If this doesn't merge cleanly into that branch, I may just copy and paste from your PR rather than merging, with due credit of course.

@aaronquantexa
Copy link
Author

Cool, I'll have a go at using DenseMatrix.

On point 2, sounds fair - I'll leave this to you as maintainer.

@aaronquantexa
Copy link
Author

If it doesn't merge easily, can remove the additional - now redundant functions - in DenseVector.scala

@jczestochowska
Copy link

related question: is subtraction between CSCMatrix and DenseVector supported? I tried - operator and all other related - operators but it seems like it's not implemented. @aaronquantexa @dlwh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aaronquantexa @dlwh @jczestochowska